Ractor-shareable JSON::Coder#897
Conversation
|
CI needs some fixing but otherwise 👍 |
| }, | ||
| 0, 0, | ||
| RUBY_TYPED_FREE_IMMEDIATELY | RUBY_TYPED_WB_PROTECTED, | ||
| RUBY_TYPED_FREE_IMMEDIATELY | RUBY_TYPED_WB_PROTECTED | RUBY_TYPED_FROZEN_SHAREABLE, |
There was a problem hiding this comment.
This seems correct because all the fields in
json/ext/json/ext/parser/parser.c
Lines 331 to 342 in 7b1fb27
are only written in
parser_config_init().However we need to make sure that's not called multiple times, so there should be a
rb_check_frozen() at the beginning of parser_config_init().
BTW bool parsing_name; seems unused.
There was a problem hiding this comment.
BTW
bool parsing_name;seems unused.
Good catch: ab5efca
There was a problem hiding this comment.
@eregon Thanks for the review! I added a call to rb_check_frozen() in cParserConfig_initialize.
>> JSON::Coder.new.freeze.instance_variable_get(:@parser_config).send(:initialize, {})
(irb):1:in 'JSON::Ext::ParserConfig#initialize': can't modify frozen JSON::Ext::ParserConfig: #<JSON::Ext::ParserConfig:0x0000000125000d40> (FrozenError)I wasn't sure whether to add a test for that, it looked like this:
coder = JSON::Coder.new.freeze
parser_config = coder.instance_variable_get(:@parser_config)
assert_instance_of JSON::Parser::Config, parser_config
assert_raise FrozenError do
parser_config.send(:initialize, {})
endBTW I just realized that the Generator::State object is already frozen in initialize, I'll actually do the same for the Parser::Config.
There was a problem hiding this comment.
We'll also need to make sure a frozen Ext::Generator::State is not modified:
>> state = JSON::Coder.new.instance_variable_get(:@state)
=> #<JSON::Ext::Generator::State:0x000000012557c7d8>
>> state.frozen?
=> true
>> state.max_nesting
=> 100
>> state.max_nesting=1
=> 1
>> state.max_nesting
=> 1
There was a problem hiding this comment.
Good catch, that's already marked as RUBY_TYPED_FROZEN_SHAREABLE but it's not correct since it can still be mutated. So I guess we need rb_check_frozen() in all methods that can mutate a Ext::Generator::State then.
For Ext::Generator::State fields which don't contain a shareable value (e.g. strings) it would break the Ractor invariant (unshareable objects must always stay in a single Ractor) and probably segfault. And it shouldn't be visible as mutable anyway if marked as RUBY_TYPED_FROZEN_SHAREABLE.
77c03ae to
7ea7a05
Compare
|
I can't repro the errors with JRuby on my machine. I don't have the same JDK but still… vs. https://github.com/ruby/json/actions/runs/19504076937/job/55825471457#step:3:39 |
7ea7a05 to
76ed0f4
Compare
Weird, I can't either. |
|
It seems somewhat transient as it doesn't happen on all platforms. |
|
The path seems correct: /Users/runner/work/json/json/lib/json/ext/generator/state.rb
/Users/runner/work/json/json/lib/json/ext/generator.jarhttps://github.com/ruby/json/actions/runs/19541236496/job/55947865782#step:5:24 |
|
Maybe it’s not worth testing this closely anyways? |
|
Yeah at first I didn't have the test at all but then when I found out that the existing |
0c34822 to
7ca198c
Compare
|
I would suggest to keep the test (it was a pretty subtle and important bug) and skip it on JRuby, it sounds like a JRuby bug to me, maybe something @headius could investigate when he has some time. |
|
Re-reading the diff, the test for |
7ca198c to
1df7523
Compare
bfcd535 to
1219ce0
Compare
|
I've added back the test for |
I want to be able to reuse an instance of
JSON::Coderacross Ractors. We need to add RUBY_TYPED_FROZEN_SHAREABLE to the parser config typed data definition and freeze the instance variables, after which it's possible to use a frozen coder across Ractors.